feat(mail): send outbound replies, minting reply tokens (HT-15) - #12
Merged
Conversation
Closes the mail-engine loop: parse → thread → store → SEND. sendReply mints the signed reply token (HT-12) into the outbound Message-ID, so a future customer reply threads back via decideThreading (HT-13) + the store (HT-14). Proven end-to-end by a round-trip test. - src/providers/email-sender.ts — EmailSender provider seam. Contract: the provider MUST transmit the engine-set Message-ID verbatim (threading depends on it); a provider that can't is unusable. - src/mail/send.ts — sendReply: app-generates the outbound thread UUID, mints the token from it, persists the outbound thread as an outbox item (delivery_status='pending'), sends, then marks sent/failed. Persist→send →mark ordering so a crash never reports a false 'sent'; retries reuse the same id/Message-ID, never re-mint (specs/mail/sending.md §3). - db migration 002 + store: delivery_status with a direction-tied CHECK; explicit-id inserts; setThreadDeliveryStatus. - specs/mail/sending.md — the token lifecycle + outbox contract. Reviewed by Codex (crypto/threading path, per standing rule) across two adversarial rounds. Fixes landed from its findings: - preserve the original send error if the failure-mark also throws (AggregateError), never swap one for the other; - the delivery_status CHECK is a cross-column direction↔status invariant, with an explicit IS NOT NULL guard (a CHECK passes on NULL, so the naive form still admitted an outbound row with NULL status — caught by a test); - setThreadDeliveryStatus is scoped to outbound rows and RETURNING-guarded to throw on a zero-row (wrong/deleted/inbound) target; - migration 002 backfills preexisting outbound rows before adding the constraint, so it upgrades a non-fresh 001 database instead of failing (covered by a throughId-staged upgrade test). Deferred with tickets: idempotency/delivery-worker (HT-16); adapter wire-level Message-ID contract tests (first real adapter). sendReply must not be wired behind a retrying caller until HT-16. 144 tests pass; typecheck + Biome clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TqG66PPZreBrj17VbAqe3b
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds the outbound email provider contract, migration-backed thread delivery states, conversation-store support, and a synchronous ChangesOutbound reply delivery
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant sendReply
participant ConversationStore
participant EmailSender
Caller->>sendReply: Submit reply
sendReply->>ConversationStore: Persist outbound thread as pending
ConversationStore-->>sendReply: Return threadId
sendReply->>EmailSender: Send engine-generated Message-ID
EmailSender-->>sendReply: Return success or failure
sendReply->>ConversationStore: Mark sent or failed
sendReply-->>Caller: Return delivery result
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/db/migrate.ts (1)
99-102: 🧹 Nitpick | 🔵 TrivialConsider non-blocking constraint validation if
threadsgrows large on real Postgres.
ADD CONSTRAINT ... CHECKvalidates every existing row while holding anACCESS EXCLUSIVElock, blocking concurrent reads/writes for the scan duration. This is a non-issue for PGlite and small tables today, but for a largethreadstable on a live Postgres deployment you may later wantADD CONSTRAINT ... NOT VALIDfollowed by a separateVALIDATE CONSTRAINT(which takes only aSHARE UPDATE EXCLUSIVElock) to avoid a write-blocking migration.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/db/migrate.ts` around lines 99 - 102, Update the migration’s threads_delivery_status_by_direction constraint creation to use ADD CONSTRAINT ... NOT VALID, then add a separate VALIDATE CONSTRAINT statement so existing rows are checked with reduced locking on live Postgres deployments.src/mail/send.test.ts (2)
116-120: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winExercise the verbatim
Referencescontract.The happy-path test verifies Message-ID and In-Reply-To forwarding but never supplies or asserts
references. Add an ordered References fixture and assert the sender receives it unchanged.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/mail/send.test.ts` around lines 116 - 120, Update the happy-path test around the sender assertions to provide an ordered References fixture in the inbound message or send input, then assert sender.sent[0].references matches that fixture exactly and in the same order, alongside the existing messageId and inReplyTo checks.
160-181: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winCover failure while marking a successful send.
The suite covers provider failure and send-plus-failed-mark aggregation, but not the path where
sender.send()resolves andsetThreadDeliveryStatus(..., 'sent')rejects. Add this case to lock down the pending-state and error behavior before retry logic is introduced.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/mail/send.test.ts` around lines 160 - 181, Add a test alongside the existing sendReply failure cases that uses a sender whose send() resolves successfully while store.setThreadDeliveryStatus(..., 'sent') rejects. Assert sendReply rejects with the marking error and verify the outbound thread remains in pending delivery status, covering the successful-send/failed-mark path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@specs/mail/sending.md`:
- Around line 100-101: The documentation incorrectly states that no token is
minted when a conversation is missing or deleted. Update the refusal statement
in the mail-sending specification to clarify that the reply token is minted
first, then discarded when appendThread rejects the conversation; only
persistence and sending are skipped.
In `@src/mail/send.ts`:
- Line 63: Update the EmailSender type import in send.ts to import from the
providers barrel module instead of the individual email-sender.js file,
following the module import convention documented in src/providers/README.md.
---
Nitpick comments:
In `@src/db/migrate.ts`:
- Around line 99-102: Update the migration’s
threads_delivery_status_by_direction constraint creation to use ADD CONSTRAINT
... NOT VALID, then add a separate VALIDATE CONSTRAINT statement so existing
rows are checked with reduced locking on live Postgres deployments.
In `@src/mail/send.test.ts`:
- Around line 116-120: Update the happy-path test around the sender assertions
to provide an ordered References fixture in the inbound message or send input,
then assert sender.sent[0].references matches that fixture exactly and in the
same order, alongside the existing messageId and inReplyTo checks.
- Around line 160-181: Add a test alongside the existing sendReply failure cases
that uses a sender whose send() resolves successfully while
store.setThreadDeliveryStatus(..., 'sent') rejects. Assert sendReply rejects
with the marking error and verify the outbound thread remains in pending
delivery status, covering the successful-send/failed-mark path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3c8ccc4c-23c5-47b1-aede-a729d012aa39
📒 Files selected for processing (10)
specs/mail/sending.mdsrc/db/migrate.test.tssrc/db/migrate.tssrc/mail/send.test.tssrc/mail/send.tssrc/providers/README.mdsrc/providers/email-sender.tssrc/providers/index.tssrc/store/conversations.test.tssrc/store/conversations.ts
…usal wording (HT-15) Address CodeRabbit on #12: - send.ts / send.test.ts: import EmailSender/OutboundEmail from src/providers (the barrel) not the individual provider file, per src/providers/README.md (Major). - specs/mail/sending.md §5: a refused (missing/deleted) conversation mints the token first and then discards it — only persistence and sending are skipped; "nothing is minted" was inaccurate (Minor). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TqG66PPZreBrj17VbAqe3b
This was referenced Jul 11, 2026
Merged
Merged
This was referenced Jul 19, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes HT-15. The fifth mail-engine increment — send — which closes the loop: parse → thread → store → send.
sendReplymints the signed reply token into the outboundMessage-ID, so a future customer reply threads back throughdecideThreading(HT-13) + the store (HT-14). A round-trip test proves the whole loop end to end.What's here
src/providers/email-sender.ts— theEmailSenderprovider seam. Contract: transmit the engine-setMessage-IDverbatim or you're unusable (threading depends on it).src/mail/send.ts—sendReply: app-generates the outbound thread UUID, mints the token from it, persists the outbound thread as an outbox item (delivery_status='pending'), sends, then markssent/failed. Persist→send→mark ordering so a crash never reports a falsesent; retries reuse the same id/Message-ID, never re-mint.delivery_statuswith a direction-tied CHECK, explicit-id inserts,setThreadDeliveryStatus.specs/mail/sending.md— the token lifecycle + outbox contract.Design + review
The id/token circularity (the outbound
Message-IDmust embed the thread's own id, but the id is the row's PK) was worked out with Codex up front (app-generate the UUID before insert), then the implementation went through two Codex adversarial rounds on this crypto/threading path. Fixes that landed from its findings:AggregateError) rather than swapping one for the other;delivery_statusCHECK is a cross-column direction↔status invariant with an explicitIS NOT NULLguard — a CHECK passes onNULL, so the naive form still admitted an outbound row with aNULLstatus (caught by a test);setThreadDeliveryStatusis scoped todirection='outbound'andRETURNING-guarded to throw on a zero-row (wrong/deleted/inbound) target;throughId-staged upgrade test).Deferred (with tickets)
sendReplyis synchronous with no dedup key, so it must not be wired behind a retrying caller until HT-16 (documented insend.ts).Message-IDcontract tests — required of the first real adapter (spec §4); there are no real adapters yet, only the interface + a fake.Testing
Against real in-memory PGlite + a fake
EmailSender: happy path (verbatimMessage-ID), the round-trip (minted id → inboundIn-Reply-To→decideThreadingappends to the right conversation/thread), send-failure marksfailed, both-errorsAggregateError, refusals (deleted/missing), the cross-column CHECK across all five cases, the direction-scoped status guard, and the 001→002 upgrade. 144 tests pass; typecheck + Biome clean.🤖 Generated with Claude Code
Summary by CodeRabbit
pending,sent,failed) with direction-scoped updates.Message-ID,In-Reply-To,References) from engine to provider.